[New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions by juan-malbeclabs · Pull Request #3206 · malbeclabs/doublezero

juan-malbeclabs · 2026-03-09T17:39:25Z

Permission rollout — stacked PR series. Review/merge order:

#3206 — enforce Permission-based authorization in existing instructions

#3942 — define topology/resource/index permission flags (3/4) — stacked on [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions #3206

#3943 — enforce topology/resource/index permission flags (4/4) — stacked on [New Permission 4/5] smartcontract: define topology/resource/index permission flags #3942

Retarget each PR's base to main as its upstream merges.
👉 You are here: #3206 (base of the follow-up stack).

Summary of Changes

Wire authorize() into the privileged instruction processors that gate on it: accesspass/{close,set} (ACCESS_PASS_ADMIN) and user/{delete,requestban} (USER_ADMIN). Each appends the caller's Permission PDA as an optional trailing account, read last by authorize(). The user owner can still delete their own account without a Permission account.
Add execute_authorized_transaction (and its _quiet variant) to the Rust SDK. They share the existing execute_transaction_inner builder, so compute-budget, preflight, and error-reporting behavior are identical to execute_transaction; the only difference is the optional trailing Permission PDA, which is resolved at most once per client (retried on transient RPC errors, then memoized).
Switch the affected SDK commands (accesspass/{close,set}, user/{delete,requestban}, tenant/delete, and the permission/* CRUD commands) to execute_authorized_transaction so the Permission account is included transparently.
Drop the retired activator authority from the USER_ADMIN/NETWORK_ADMIN/MULTICAST_ADMIN legacy fallbacks. ACCESS_PASS_ADMIN legacy authority (foundation + sentinel + feed) is applied uniformly across its instructions and documented in authorize.rs.

Testing Verification

New DZClient-level tests assert the assembled instruction list carries the protocol-max compute-budget/heap-frame requests and that the Permission account is appended after payer+system, with and without an on-chain permission account.
authorize unit tests cover the legacy fallbacks (including the activator now being denied for the admin roles) and the Permission-account path (PDA, ownership, discriminator, status, and flag checks).
cargo test -p doublezero_sdk and cargo test -p doublezero-serviceability pass.

Related RFC/series: the Permission account and authorize() mechanism land in the earlier PRs of this series; this PR enforces them in existing instructions.

ben-dz

The on-chain authorize() wiring across the 4 migrated processors (accesspass close/set, user delete/requestban) is sound and not bypassable. The blocking issue is in the Rust SDK: execute_transaction was rerouted through a new build_and_send helper that silently drops the protocol-max compute-budget and heap-frame instructions for every transaction in the SDK (not just the permission ones), and also changes preflight (skip_preflight → preflight-on) and error-reporting behavior. A second execute_authorized_transaction_quiet is dead code that appends the Permission account in the wrong position. None of the new SDK behavior is covered by tests. Also flagging legacy-fallback authorization expansions (sentinel can now close access passes; activator can now ban/delete users) to confirm intent. C1 must be fixed before merge.

M2 (Medium): No test exercises the assembled transaction. SDK command tests only swap expect_execute_transaction → expect_execute_authorized_transaction on the automock, which short-circuits build_and_send. Nothing verifies compute-budget presence, the conditional permission append, the get_account probe, or the trailing position of the Permission account — the exact gap hiding C1 and H2. Add a DZClient-level test asserting the final Vec/Vec with and without an on-chain permission account.
L2 (Low): Partial migration — only 4 instructions are migrated to authorize(); user/create, user/create_subscribe, user/ban, user/closeaccount, multicastgroup/subscribe, and device/link/tenant CRUD still use inline allowlist checks. Not a regression (verified all accounts.len()-sensitive unmigrated processors still use the non-appending execute_transaction, so no account-count collision). Keep the authorize.rs legacy-fallback table in sync with the unmigrated processors as migration continues, and align the PR summary with the diff (it lists more 'wired' instructions than the diff actually touches).

Findings not anchored to the current diff:

smartcontract/programs/doublezero-serviceability/src/processors/accesspass/set.rs:70 — medium: M4 (Medium): fragile positional optional-account detection. accounts.len() >= 7 infers tenant presence; it is correct alongside the appended Permission account only because tenant adds 2 accounts and permission adds 1 (no-tenant+perm=6 <7; tenant+perm=8 >=7). Any future trailing-layout change silently misclassifies an account as a tenant. Encode optional-account presence in instruction args (as user/delete does), and/or validate the candidate Permission account's AccountType discriminator inside authorize(); add tests for 6- and 8-account shapes.
smartcontract/programs/doublezero-serviceability/src/authorize.rs:63 — low: L1 (Low): check ordering. data_is_empty (line 60) is checked before owner == program_id (line 63). Safe because the PDA-address equality at line 57 already blocks account substitution. Optional hardening: assert owner == program_id immediately after the PDA check.

juan-malbeclabs · 2026-06-29T19:44:53Z

Review feedback addressed (`4660f08`)

Thanks for the thorough review. Inline replies cover C1, H1, H2, H3, M1, M3, L3, and the changelog note. Summary findings:

M2 — Added DZClient-level tests asserting the assembled instruction list: compute-budget instructions are present, and the Permission account is appended as the trailing account both with and without an on-chain permission account (assemble_instructions_tests in client.rs). The build/send path was extracted into a pure assemble_instructions so it is testable without RPC.
M4 — authorize() already rejects a misclassified account (it validates the PDA address, program ownership, and the AccountType::Permission discriminator), so the trailing Permission account can never be read as a tenant. Documented the exact accounts.len() arithmetic invariant in accesspass/set.rs. Encoding tenant presence in instruction args is a worthwhile follow-up but is an ABI change, so left out of this PR.
L1 — Reordered the checks in authorize() so program ownership is verified immediately after the PDA-address check, before inspecting account data.
L2 — The activator broadening is removed (see M1); the activator has been retired from the system. PR description updated to match the actual (rebased) diff.

elitegreg · 2026-06-29T21:07:08Z

accesspass/close.rs — sentinel can now close any access pass

Switching close to the ACCESS_PASS_ADMIN legacy set (foundation || sentinel || feed) broadens the close authority to include the sentinel authority, which the PR description notes is intentional. One thing I'd like to confirm: the existing own-only guard a bit further down only restricts the feed authority:

// Feed authority can only close access passes they own
if globalstate.feed_authority_pk == *payer_account.key
    && accesspass.owner != *payer_account.key
{
    return Err(DoubleZeroError::NotAllowed.into());
}

Since sentinel now passes the same authorization check but isn't covered by this guard, sentinel can close any access pass, not just ones it owns. Is that the intended scope for sentinel, or should the own-only restriction extend to it as well?

Feedback was addressed.

juan-malbeclabs · 2026-06-30T12:23:54Z

@elitegreg Good catch, and yes, the broadening to sentinel is intentional.

To answer the question directly: the own-only guard should not extend to sentinel. It's a feed-authority-specific carve-out by design, and the same pattern holds in set.rs (the update path guards only on feed_authority_pk as well). The intent is two tiers inside ACCESS_PASS_ADMIN:

foundation + sentinel → operator-tier admins, unrestricted scope
feed → delegated authority, limited to access passes it owns

The reason own-only makes sense for feed but not for sentinel is ownership semantics: owner is set to the payer at creation (set.rs:197), and feed authority owns the passes it creates. Sentinel is a monitoring/enforcement authority — it doesn't create passes for itself, so it would essentially never be the owner. Applying the own-only guard to it would mean it could close almost nothing, which defeats the purpose of granting it ACCESS_PASS_ADMIN.

So the real trust decision here is whether sentinel should hold ACCESS_PASS_ADMIN at all (which is the intended scope) rather than scoping it down with the feed guard. Leaving sentinel unrestricted is the consistent and intended behavior. No change planned here.

…ants

…anism

…tructions

- Add execute_authorized_transaction_quiet to DoubleZeroClient trait and DZClient impl, restoring quiet mode for ban and closeaccount commands that was lost when switching to execute_authorized_transaction - Restore onchain allocation support in CreateSubscribeUserCommand SDK command (feature-flag-gated ResourceExtension PDA logic removed in permission enforcement refactor) - Restore atomic path tests and fixture resource extension PDA fields in create_subscribe_user_test.rs

- SDK: route execute_transaction and execute_authorized_transaction through a single execute_transaction_inner, restoring the protocol-max compute-budget / heap-frame requests, skip_preflight, and structured error handling that the build_and_send detour had dropped for the whole SDK (C1/H1). - SDK: append the Permission PDA as the trailing account so it stays after payer+system, matching authorize()'s read order; the quiet authorized variant now shares the same builder (H2/H3/L3). - SDK: memoize the Permission PDA lookup and retry it on transient RPC errors, removing the per-send un-retried get_account (M3). - SDK: add DZClient-level tests asserting compute-budget presence and the trailing Permission account, with and without a permission account (M2). - authorize: drop the retired activator authority from USER_ADMIN/NETWORK_ADMIN/ MULTICAST_ADMIN legacy fallbacks; document the ACCESS_PASS_ADMIN expansion (M1). - authorize: verify program ownership before inspecting account data (L1). - accesspass/set: document the optional-account count invariant (M4). - CHANGELOG: correct the SDK entry to match the unified builder.

…delete/ban DeleteUserCommand and RequestBanUserCommand authorize the final instruction with USER_ADMIN, but first strip the user's multicast roles through UpdateMulticastGroupRoles, whose processor only accepted the access-pass owner or a foundation member. A USER_ADMIN-only payer was therefore blocked on the prerequisite cleanup. Allow the role-update processor to accept USER_ADMIN for the removal-only case (no roles being granted), and route UpdateMulticastGroupRolesCommand through execute_authorized_transaction so the payer's Permission account is appended when present. Adding roles still requires the owner or foundation.

…to-injection The SDK auto-appends the payer's Permission PDA whenever it exists on-chain, so a foundation member whose own Permission account was suspended, under-privileged, or uninitialized was routed through the Some branch of authorize() and denied the PERMISSION_ADMIN instruction needed to repair it — the None-branch lockout fallback never ran. Extract foundation_permission_recovery() and apply it in the Some branch too: when the supplied Permission account does not grant the flag, a foundation member requesting PERMISSION_ADMIN is still allowed. PDA-address and ownership checks remain hard errors; recovery is scoped to PERMISSION_ADMIN only.

…rmission flags (#3942) > **Permission rollout — stacked PR series.** Review/merge order: > 1. [#3206 — enforce Permission-based authorization in existing instructions](#3206) > 2. [#3942 — define topology/resource/index permission flags (3/4)](#3942) — stacked on #3206 > 3. [#3943 — enforce topology/resource/index permission flags (4/4)](#3943) — stacked on #3942 > > Retarget each PR's base to `main` as its upstream merges. > **👉 You are here: #3942 (PR 3/4).** ## Summary - Adds three permission flags — `TOPOLOGY_ADMIN` (`1<<15`), `RESOURCE_ADMIN` (`1<<16`), `INDEX_ADMIN` (`1<<17`) — so segment-routing topologies, `ResourceExtension` accounts, and internal `Index` accounts can be delegated without granting the broad `FOUNDATION` flag (today they are foundation-only). - Maps each flag to the `foundation_allowlist` in `authorize()`'s legacy path, so authorization behavior is unchanged until a `Permission` account is supplied. - Exposes the names (`topology-admin` / `resource-admin` / `index-admin`) in the serviceability CLI for `permission set --add` / `--remove`, and adds matching constants to the Go, TypeScript, and Python SDKs. - Documents the flags in `PERMISSION.md` and the serviceability `README.md`. This PR is definition-only; processor enforcement lands in the follow-up 4/4. Stacked on #3206. ## Testing Verification - `authorize()` legacy-mapping unit tests for each new flag (allowed via a foundation member, denied for others). - CLI permission name↔bitmask round-trip tests cover the three new names. Reference: `smartcontract/programs/doublezero-serviceability/PERMISSION.md`

…ermission flags (#3943) > **Permission rollout — stacked PR series.** Review/merge order: > 1. [#3206 — enforce Permission-based authorization in existing instructions](#3206) > 2. [#3942 — define topology/resource/index permission flags (3/4)](#3942) — stacked on #3206 > 3. [#3943 — enforce topology/resource/index permission flags (4/4)](#3943) — stacked on #3942 > > Retarget each PR's base to `main` as its upstream merges. > **👉 You are here: #3943 (PR 4/4).** ## Summary - Replaces the direct `foundation_allowlist` checks with `authorize()` in topology (`create` / `delete` / `clear` / `assign-node-segments`) → `TOPOLOGY_ADMIN`, resource (`create` / `allocate` / `deallocate` / `close`) → `RESOURCE_ADMIN`, and index (`create` / `delete`) → `INDEX_ADMIN`. - The `Permission` account is read as the optional trailing account; the variable-length `clear` and `assign-node-segments` layouts detect it by PDA match before consuming their link/device lists. - Backward compatible: with no `Permission` account the legacy foundation path still applies, so existing callers (controlplane, current SDK) keep working. - Switches the corresponding SDK commands to `execute_authorized_transaction` so the payer's `Permission` PDA is appended when it exists onchain. **Behavior change:** the topology instructions now reject unauthorized callers with `NotAllowed` (`Custom(8)`) instead of `Unauthorized` (`Custom(22)`), since `authorize()` is the single rejection path; affected tests updated. Depends on the 3/4 PR (stacked). ## Testing Verification - New end-to-end test `test_topology_create_with_permission_account_allowed`: a non-foundation key holding `TOPOLOGY_ADMIN` creates a topology via its `Permission` account — exercises the new authorization path through a real processor. - topology / index / resource / permission integration suites pass; topology error-code assertions updated `Unauthorized` → `NotAllowed`. Reference: `smartcontract/programs/doublezero-serviceability/PERMISSION.md`

juan-malbeclabs requested a review from elitegreg as a code owner March 9, 2026 17:39

juan-malbeclabs force-pushed the jo/permission-enforcement branch from fcc211f to cb87d98 Compare March 9, 2026 17:43

juan-malbeclabs changed the base branch from jo/permission-crud to jo/permission-crud-sdk-cli March 9, 2026 17:46

juan-malbeclabs changed the title ~~smartcontract: enforce Permission-based authorization in existing instructions~~ [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions Mar 9, 2026

juan-malbeclabs force-pushed the jo/permission-crud-sdk-cli branch from 53175e4 to 7c2c64c Compare March 10, 2026 16:01

juan-malbeclabs force-pushed the jo/permission-enforcement branch from cb87d98 to a6b59b4 Compare March 10, 2026 16:03

juan-malbeclabs force-pushed the jo/permission-crud-sdk-cli branch from 7c2c64c to 5bdc49e Compare March 11, 2026 15:11

juan-malbeclabs force-pushed the jo/permission-enforcement branch from a6b59b4 to cdf8668 Compare March 11, 2026 15:14

elitegreg reviewed Mar 12, 2026

View reviewed changes

Comment thread smartcontract/programs/doublezero-serviceability/src/processors/multicastgroup/subscribe.rs Outdated

elitegreg requested changes Mar 12, 2026

View reviewed changes

juan-malbeclabs force-pushed the jo/permission-crud-sdk-cli branch from de2986e to 426ea94 Compare March 12, 2026 19:34

Base automatically changed from jo/permission-crud-sdk-cli to main March 12, 2026 19:47

juan-malbeclabs force-pushed the jo/permission-enforcement branch 2 times, most recently from e63b15f to 8bc4aee Compare March 13, 2026 13:18

elitegreg requested changes Mar 13, 2026

View reviewed changes

juan-malbeclabs linked an issue Mar 13, 2026 that may be closed by this pull request

serviceability: Permission account model for scalable, granular access control #3183

Closed

juan-malbeclabs force-pushed the jo/permission-enforcement branch 2 times, most recently from 48a5b44 to 5504a1f Compare March 16, 2026 17:18

juan-malbeclabs changed the title ~~[New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions~~ [New Permission 3/3] smartcontract: enforce Permission-based authorization in existing instructions Jun 26, 2026

juan-malbeclabs force-pushed the jo/permission-enforcement branch 2 times, most recently from 1a8d0db to f9a4ff5 Compare June 26, 2026 21:22

juan-malbeclabs mentioned this pull request Jun 26, 2026

[New Permission 4/5] smartcontract: define topology/resource/index permission flags #3942

Merged

juan-malbeclabs closed this Jun 26, 2026

juan-malbeclabs reopened this Jun 26, 2026

juan-malbeclabs mentioned this pull request Jun 26, 2026

[New Permission 5/5] smartcontract: enforce topology/resource/index permission flags #3943

Merged

juan-malbeclabs changed the title ~~[New Permission 3/3] smartcontract: enforce Permission-based authorization in existing instructions~~ [New Permission 3/5] smartcontract: enforce Permission-based authorization in existing instructions Jun 27, 2026

juan-malbeclabs force-pushed the jo/permission-enforcement branch from f9a4ff5 to 8e8e6ac Compare June 29, 2026 17:41

juan-malbeclabs enabled auto-merge (squash) June 29, 2026 17:42

juan-malbeclabs closed this Jun 29, 2026

auto-merge was automatically disabled June 29, 2026 18:27
Pull request was closed

juan-malbeclabs reopened this Jun 29, 2026

ben-dz previously requested changes Jun 29, 2026

View reviewed changes

vihu reviewed Jun 29, 2026

View reviewed changes

Comment thread smartcontract/sdk/rs/src/commands/user/delete.rs

vihu reviewed Jun 29, 2026

View reviewed changes

Comment thread smartcontract/programs/doublezero-serviceability/src/authorize.rs

ben-dz self-requested a review June 30, 2026 01:13

juan-malbeclabs added 7 commits June 30, 2026 13:11

smartcontract: scaffold Permission account state and instruction vari…

fb37e06

…ants

smartcontract: implement Permission account CRUD and authorize() mech…

4c489d7

…anism

smartcontract: enforce Permission-based authorization in existing ins…

97fc8bf

…tructions

juan-malbeclabs force-pushed the jo/permission-enforcement branch from 20dcbed to 1ccffdb Compare June 30, 2026 13:11

elitegreg self-requested a review June 30, 2026 15:08

elitegreg approved these changes Jun 30, 2026

View reviewed changes

juan-malbeclabs merged commit 1855fa3 into main Jun 30, 2026
34 checks passed

juan-malbeclabs deleted the jo/permission-enforcement branch June 30, 2026 15:14

bgm-malbeclabs mentioned this pull request Jul 1, 2026

smartcontract: enforce Permission flags on multicast allowlist and subscribe/create-user handlers #3966

Open

Uh oh!

Conversation

juan-malbeclabs commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of Changes

Testing Verification

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben-dz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juan-malbeclabs commented Jun 29, 2026

Review feedback addressed (4660f08)

Uh oh!

Uh oh!

Uh oh!

elitegreg commented Jun 29, 2026

Uh oh!

juan-malbeclabs commented Jun 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

juan-malbeclabs commented Mar 9, 2026 •

edited

Loading

Review feedback addressed (`4660f08`)